Move API docs redirects to CloudFront #366
Conversation
We've hit the limits of what S3 bucket redirections allow, so we're serving the 404 page with a `200 OK` status code - breaking HTTP, and making [AI] crawlers go crazy coming up with paths. We can now move that redirection logic to CloudFront, and be correct on our status codes.
|
Deployment note: we still need to "apply" the function to the current CloudFront distribution for it to take effect. Also, we'll manually drop the old S3 bucket rules. |
docs/aws-cloudfront-redirect.js
Outdated
| if(requestPath.startsWith('/api/latest/')) { | ||
| redirectUri = requestPath.replace('/api/latest/', '/api/${CRYSTAL_VERSION}/') | ||
| } else if(requestPath.startsWith('/api/') && !(requestPath.startsWith('/api/0') || requestPath.startsWith('/api/1'))) { | ||
| redirectUri = requestPath.replace('/api/', '/api/${CRYSTAL_VERSION}/') | ||
| } |
There was a problem hiding this comment.
issue: I think we also need to pass through paths with the /api/master/ prefix (for the HEAD version of the docs) and /api/versions.json (metadata).
There was a problem hiding this comment.
In the current s3 setup, we also have the following redirects which we might want to incorporate into cloudfront:
/api->/api/(302)/api/->/api/latest(301)/api/index.html->/api/latest(301)/api/latestto/api/latest/(301)
Maybe they should all redirect directly to /api/${CRYSTAL_VERSION}/ without intermediaries and all be 302s? Or 301s to /api/latest/...
Then we could delete the files index.html and latest in the s3 bucket.
There was a problem hiding this comment.
Yes, they can all be 301 to /api/latest/ (trailing slash), which would avoid some redirects and make the move explicit 👍
There was a problem hiding this comment.
Oohhh! Good catch.
The S3 rules don't mention anything about /api/master/ and /api/versions.json, and I was scratching my head trying to understand why does this work different, then?. But that's because S3 rules apply when S3 resolves to a 404 error. This CloudFront function was meant (so far, at least) to be applied before hitting S3.
My first instinct is to keep this function as simple as possible: move it to after the request went to S3, and only work with 404's (as we're doing now with S3); and keep the few redirects that @straight-shoota mentions that are files in the bucket (ie, not S3 "logic").
But if you have a strong reason to move all the logic to this function, I can do that too.
There was a problem hiding this comment.
Yeah, I was also expecting this function to go before S3.
Because there's no point in bothering S3 when we can easily tell that the request wouldn't match anything.
But I'm not determined on whether that's actually better or not. I suppose most legitimate requests should not hit a 404 anyway. So maybe it's more efficient to have the rewrite only after S3.
There was a problem hiding this comment.
I don't think so. But the different versions are easily accessible from the version selector on each API doc page.
There was a problem hiding this comment.
I meant an internal doc to avoid situations where it's only in someone's 🧠.
There was a problem hiding this comment.
Ah yeah, we probably should write that down somewhere. Although the CI workflows also document what's happening.
Btw. master is actually master, not nightly. It updates on every commit to the master branch, not just once per day.
There was a problem hiding this comment.
From CloudFront - Restrictions on all edge functions:
CloudFront doesn't invoke edge functions for viewer response events when the origin returns HTTP status code 400 or higher.
🤦
So we can't say "send the request to S3, and we'll do our thing if that fails" 😞
Don't know why they put this limitation in place, but it changes the framing of the function. We'll need to apply the redirects before trying to hit the bucket.
There was a problem hiding this comment.
Damn. Well, the rules are static and small. It's doable to hardcode them.
Co-authored-by: Johannes Müller <[email protected]>
The S3 bucket had two empty files that were redirection-only: - api/index.html --> /api/latest - api/latest --> /api/latest/ We now add them to the function's check.
CloudFront requires to publish the new version, and we have to get their ETag each time.
CloudFront doesn't run viewer response functions when the origin responds a 4xx or 5xx, so we have to hard-code which requests can pass straight to the origin. This function will need to get updated if we add extra files to the `/api/` directory, such as a 2.x release, or other new files - I predict this will be the source of a few bugs in the future.
|
OK, I think the function is now working OK. I've set Feel free to test every URL you can think of, so we merge this and publish on production. |
|
The API sounds good to me. |
We've hit the limits of what S3 bucket redirections allow, so we're serving the 404 page with a
200 OKstatus code - breaking HTTP, and making [AI] crawlers go crazy coming up with paths.We can now move that redirection logic to CloudFront, and be correct on our status codes.